fix(mobile): hydrate and refresh relay presence snapshots - #7382
Conversation
🔐 Codex Security Review
|
|
@buzz-security-review a8e8028 |
Independent presence readiness evidence — 2026-09-07Verified current range:
Advisory security: unavailable, not cleanThe authorized exact-range advisory finished cancelled at 15:54:56 UTC. Its annotation states: “The job has exceeded the maximum execution time of 40m0s.” Authorization/checkout/range pin passed, but review never completed; salvage/post did not run, zero artifacts were published, and the review-job log endpoint returned 404. No findings were produced for inspection. No manual cancellation or blind retry was performed. Next actor/action: the maintainer of the trusted Codex Security Review workflow must resolve the runner/action timeout/output loss, then rerun the authorized current range. Resume advisory sign-off only when actual current-range output is posted and findings have been reviewed. This is an external advisory limitation, not a failed presence test or a claim that security review passed. Technical review found no substantive mobile defect: snapshot/live overlap, per-key revisions, generation fences, invalidation on transport/account/community/lifecycle changes, and bounded 60-second polling were checked. Unknown rendering is deliberately the separate stacked UI PR #7383. Presence is conversational availability, not substrate health. Native relay-backed foreground/background/reconnect validation remains unavailable on this host (no Xcode/simulator). Widget renders are not native-runtime evidence. Both PRs remain drafts; no merge/readiness-state change was made. Durable evidence: |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Reviewing exact head a8e8028bfb9bb67de2db03ed448839e2f3ab2b0d against base 3c7f288c60d67df78577b237e27c3dfc8831aaa1, I found two user-visible correctness defects that need to be resolved before merge.
-
Transient snapshot failures are rendered as authoritative Offline.
PresenceCacheNotifier._refreshremoves requested keys when the query fails (mobile/lib/features/profile/presence_cache_provider.dart:163-195), while each production consumer converts an absent key directly to offline (mobile/lib/features/profile/user_profile_sheet.dart:56-57,mobile/lib/features/channels/channel_detail_page/app_bar.dart:218-250, andmobile/lib/features/channels/channels_page/channel_tile.dart:177-182). A previously online/away person therefore flips visibly—and in profile accessibility semantics, audibly—to Offline during relay, Redis, auth, or network failure. The new test asserts that the failed refresh empties the helper cache, but does not exercise a widget consumer, so it codifies the state loss without protecting the user-facing contract.Required: preserve unknown/stale explicitly through the renderer (for example, neutral/hidden/loading, or retained last-known state marked stale), and add widget regression coverage for both no observation and known-online → failed refresh. Neither case may claim Offline without evidence.
-
Snapshot freshness is not joined to live-event ordering. Snapshot settlement selects the latest result but does not write its
createdAtinto_timestamps(mobile/lib/features/profile/presence_cache_provider.dart:176-198). The next live event is consequently compared against zero in_handlePresenceEvent(:137-149). A production-notifier regression probe at this head demonstrated that snapshotonline @ 20followed by delayed liveoffline @ 10yieldsoffline(expectedonline). HTTP snapshot and WebSocket delivery are independent, so delayed pre-snapshot delivery can regress visible presence until another heartbeat or the next refresh.Required: use one monotonic freshness policy across snapshot and live settlement, define equal-second conflict behavior, account for relay-synthesized snapshot timestamps versus signer clock skew, and add the deterministic snapshot-20/live-10 regression.
Validation at this exact head: just mobile-check passed; the full just mobile-test passed 2,083 tests; focused shipped presence suites passed 17 tests; git diff --check passed. The adversarial stale-order probe failed as described and was then removed, leaving its review worktree clean. Identity/tenant boundaries, subscribe-before-snapshot fencing, scope reset, cancellation/disposal, retry, batching, and Redis tenant TTL storage otherwise held under source/test review. Green gates do not cover the two production seams above.
Confidence gaps, not additional author defects: no native iOS foreground/background/reconnect or accessibility journey was observed, and the mobile client relies on authenticated transport plus server validation/synthesis rather than locally verifying inbound Nostr signatures. Native relay-backed verification should follow the fixes when a simulator/device-capable owner is available.
loganj
left a comment
loganj
left a comment
There was a problem hiding this comment.
Disposition for review 5142925666 at head a8e8028bf — both items are addressed in this stack; this branch itself is intentionally left unchanged so each stack delta stays small and reviewable.
-
Transient snapshot failures rendered as authoritative Offline. The consumer-side fix is in #7383 (this PR's descendant): it removes the null→offline mapping at all three renderers, hides unknown dots/notches, and labels unknown presence explicitly. Landing #7382 together with #7383 ships the unknown semantics without the intermediate false-Offline UI, so I have not duplicated those renderer changes onto this branch.
-
Snapshot freshness not joined to live-event ordering. Fixed in #7526, a focused follow-up based on #7383's branch rather than further growth here. It records each settled snapshot's observation second, never compares the relay-synthesized snapshot timestamp against signer event time directly (snapshot records carry the relay clock at synthesis; live kind:20001 events carry the subject's signer clock), and confirms a live event older than the observation second against a fresh snapshot — the relay ingests presence before fan-out, so that observation always reflects the event or a newer one. Equal-second conflicts settle last-arrival-wins, and a bounded event-id memory keeps redelivered ambiguous events from re-arming confirmations. The deterministic snapshot-20/live-10 regression is included, plus lagging-clock confirm, equal-second, and redelivery bounds; full mobile suite 2090/2090 at its head 08d20f9.
Happy to reshape the stack if maintainers prefer a different ordering.
## Summary
- Reject kind:20001 presence events with `OK false` / `error: presence
storage unavailable` when Redis SET or DEL fails, before publishing,
local fan-out, or local-event marking.
- Preserve the producer contract needed by snapshot-confirming
consumers: delivered live presence must follow successful mutation of
the Redis state read by snapshots.
- Classify those backend rejections with the existing `IngestError`
taxonomy so a presence storage outage counts as
`buzz_events_rejected_total{transport="ws",reason="error"}`, not client
`reason="invalid"`; genuine client-input refusals (verification failure,
membership gates) stay `invalid`, and every wire message is an unchanged
fixed sanitized string (review follow-up, no protocol wording change).
- Add actual `handle_event` integration coverage for rejected
online/offline transitions, healthy online→offline
accepted/stored/fanned-out behavior, and the rejection-counter routing
on storage failure with an invalid-signature control.
This is standalone on main; it does not depend on the mobile
implementation. Deploy this relay prerequisite before relying on #7526's
snapshot-confirmation policy. Existing
pubsub-failure-after-successful-storage behavior and disconnect TTL
cleanup are deliberately unchanged. A storage error may be an ambiguous
write outcome, not a rollback guarantee; the rejected event is not
published by this handler. Clients may retry the generic `error:`
rejection. Desktop's 60s heartbeat retries non-offline presence, not
every explicit offline transition.
### Related issue
Addresses the relay prerequisite identified in [#7526 review
5157607827](#7526 (review)).
Searched open presence/storage PRs; no duplicate relay storage-error
rejection fix found. #7382/#7383/#7526 heads and bases are unchanged.
### Testing
Exact head: `389174df29cc02d0f885c03209eff661d8bb2ec0` (+380/-13; 393
total), one commit `389174df2` on top of the reviewed `c031d6eb1`
(DCO-signed; base `bfc384855889432df4a333a0edf3080f332ee169` unchanged).
- PASS: `cargo fmt --all -- --check`, `cargo clippy -p buzz-relay
--all-targets -- -D warnings`, `git diff --check`, `just
file-size-check`, PostgreSQL discovery validation — all run at the exact
final head with a clean tree before and after.
- PASS: documented native `scripts/postgres-test-run.sh -p buzz-relay
--lib --tests`: **89/89** actual integration tests, including the four
presence cases (online/offline storage rejection, healthy
online→offline, and the new rejection-classification case). Owned
PostgreSQL 17/Redis on isolated loopback ports, schema plus
reconciliation applied; no shared development database.
- PASS: explicit `cargo test -p buzz-relay presence_storage -- --ignored
--nocapture`: **4/4**, not skipped.
- Full isolated relay crate suite at the final head (`cargo nextest run
-p buzz-relay --lib --tests`): **1062 run: 1062 passed, 94 skipped**.
The previously failing
`api::mesh_demo::tests::demo_join_forwarded_arm_round_trips_echo` passed
in this run (1.5s); it is a known timing-sensitive main baseline failure
tracked open in #7140 and untouched by this PR, so this single passing
run is reported as-is and does not claim environmental clearance or
close #7140. No full-suite-green claim is made beyond this run.
- Mobile is untouched; #7526's existing 2090-test/format/analyze
evidence remains scoped to its unchanged head. Its separate Desktop
Smoke E2E (2) failure remains red; no CI retries requested.
[Production-seam regression
coverage](https://github.com/block/buzz/blob/389174df29cc02d0f885c03209eff661d8bb2ec0/crates/buzz-relay/src/handlers/event.rs#L1491-L1803):
the metric case drives real `handle_event` traffic against a genuinely
dead Redis endpoint with a seeded active PostgreSQL community and a
registered presence watcher, asserts the storage rejection counts
`reason="error"` while a tampered-signature control through the same
dispatcher arm stays `reason="invalid"`, and re-asserts the rejected
ACK, no fan-out, and no local-event marker. Counter assertions use a
thread-local recorder guard held across `.await` points (the buzz-db
counter-test convention) inside the per-process nextest postgres-ci
lane, so no parallel test can race the counter snapshot.
No UI change or screenshot. Local logs and reproducible service/gate
scripts are retained under
`WORK_LOGS/MOBILE_FEEDBACK_PRESENCE_20260909/relay_prerequisite/metric_correction/`
in the engineering workspace. This PR is a review candidate, not merge
clearance.
Causal checks: restoring only the pre-fix production mutation block
makes both original rejection tests fail (`OK true` instead of `false`);
healthy success still passes. Reverting only the typed classification
(mapping the ephemeral `Internal` arm back to `invalid`) makes the new
metric regression fail with the outage counted as `[("ws","invalid",2)]`
instead of `[("ws","error",1),("ws","invalid",1)]`. The unchanged mesh
echo case also failed 504/200 with the main-production block restored in
the prior run, supporting its separation from this change without
claiming environmental clearance. Candidate source restored
byte-for-byte after each mutation. Repository-wide `just ci` was not
rerun; the scoped relay gates above are the new evidence.
---------
Signed-off-by: Logan Johnson <loganj@squareup.com>
a8e8028 to
2e3eae6
Compare
|
Both blocking items from review 5142925666 are now included directly in this PR at 2e3eae681, based on main
Complete main → head diff is 8 Mobile files, +802/−96 = 898 including four test files. Local evidence is reused by exact whole-Mobile-subtree identity with the independently reviewed/tested candidate: 2,113 passed / 0 failed, six targeted presence/geometry controls, analyzer clean, formatter 552 files unchanged. These were not rerun on this new commit; intervening main changes touch no Mobile validation inputs. This is not a new CI success claim. Native device/live-relay/accessibility journeys and actual relay rollout remain unverified. Requested re-review from @jedwards27 through GitHub's review-request API (accepted and verified pending). Request delivery is not approval. |
Signed-off-by: Logan Johnson <loganj@squareup.com>
2e3eae6 to
3ff2f29
Compare
|
@jedwards27 — requesting exact-new-head re-review after independent review and parent-first publication. Head: This governing root now directly owns both original findings from review 5142925666: failed/absent observations render Unknown (hidden unconfirmed dots, one truthful profile accessibility label), and snapshot/live ambiguity is confirmed rather than immediately regressing settled state. The equality defect identified in #7526 reviews 5174631081 / 5174636673 is repaired here with Freshness/producer evidence: the root increments the per-key revision before queuing ambiguous evidence. A pre-event in-flight response cannot settle that key; the pending loop issues a distinct subsequent query. Idle coalescing groups keys, not response evidence. Local evidence (one composed leaf execution, not separate 2126-test runs per ancestor): immutable leaf Causal rollback: minimal Outstanding: initial exact-head CI is pending/in progress, not green; old-head CI is not evidence for these commits. Re-review and required new-head CI remain merge gates. No new Rust/Desktop runtime, simulator/native or live deployment acceptance is claimed. Structural #7532 ancestry is not a deployed relay receipt: release ownership must still establish the deployed producer guard and relay-before-client rollout/native acceptance. No merge or release clearance is requested by this evidence post. |
jedwards27
left a comment
jedwards27
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Reviewed: 813bbd14121edacc6cb4733301a3af12131aa10e..3ff2f290bac41811fe2db0b141961148740ebce5 (exact head 3ff2f290bac41811fe2db0b141961148740ebce5)
Risk: high — authenticated relay snapshots, live-event ordering, lifecycle recovery, and visible availability state.
Behavior/contracts traced: mobile rendering/accessibility; Riverpod session/lifecycle/community scoping; NIP-98 /query; Redis presence synthesis; persistence-before-fanout ordering; retry, expiry, batching, and stale-completion fences.
Blocking finding
[P2] A successful all-offline snapshot has no observation watermark, so delayed pre-snapshot live evidence can resurrect stale Online/Away.
mobile/lib/features/profile/presence_cache_provider.dart:188-214 computes _snapshotAt only from returned events. For a successful empty response, observedAt remains 0, but the cache authoritatively writes offline. A subsequently delivered live event therefore bypasses the confirmation branch at :154-163 and immediately changes visible state.
That ordering is permitted by the production relay contract: presence ingestion mutates Redis before global fan-out (crates/buzz-relay/src/handlers/event.rs:851-895), while the HTTP snapshot independently reads Redis and returns an authoritative empty result after lease absence (crates/buzz-relay/src/api/bridge.rs:2285-2301). An ingest can persist, its fan-out can be delayed, the lease can expire, an empty snapshot can settle Offline, and then the old fan-out can arrive. Independent production-notifier probes reproduced [] → Offline; delayed online@10 → Online at this head. The client has no relay observation token that distinguishes this stale delivery from genuinely post-snapshot evidence.
Consequence: a person authoritatively observed Offline can be displayed Online/Away from stale transport delivery until the next 60-second poll.
Author action: give every successful snapshot—including an entirely empty batch—a relay-issued observation watermark or equivalent safe ordering token, route at/below-watermark live evidence through fresh confirmation, and add a deterministic empty-snapshot → delayed-live regression. Do not substitute client response time unless its cross-clock and ordering semantics are established.
Verification owner: author for the protocol/client fix and causal regression; reviewer for exact-new-head relay-ordering re-review.
Re-tested prior findings
- Unknown/failure semantics are repaired. Failures remove evidence (
presence_cache_provider.dart:206-208); the DM header renders Unknown without a badge, the tile hides its dot, and the profile owns onePresence: Unknownsemantic label (app_bar.dart:247-293,channel_tile.dart:177-231,user_profile_sheet.dart:341-378). No further author action. - Non-empty and equal-second ordering are repaired. Live evidence revision-fences in-flight queries, snapshot synthesis time is retained, and
<=sends equal-second evidence through confirmation (presence_cache_provider.dart:154-163,188-214). Changing<=to<caused both equal-second state regressions to fail atpresence_snapshot_test.dart:117. No further action beyond the empty-snapshot hole. - No additional material identity/community, retry, lifecycle, or stale-completion defect was found in the traced paths.
Validation
At matching clean head 3ff2f290bac41811fe2db0b141961148740ebce5:
just mobile-check— PASS; formatter 552 files / 0 changed, analyzer clean.- Full
just mobile-test— PASS independently; 2,116 tests. - Focused changed suites — PASS independently; provider suites 21/21 and three changed suites 280 tests.
- Reviewer run:
flutter test --no-pub test/features/profile/presence_snapshot_test.dart test/features/profile/presence_cache_provider_test.dart— PASS, 21 tests. - Causal equality mutation (
<=→<) — FAIL as intended in both equal-second cases; clean source restored. git diff --check— PASS. Exact-head Mobile CI and required rollups — PASS.
Manual/native evidence: none.
Residual risk / external gates: native iOS foreground/background/reconnect and VoiceOver observation remain reviewer/release-owner confidence gaps; no author action solely for those gaps. The exact-range Codex advisory reports review required/skipped; that is an external workflow gate, not evidence of a code defect.
2 participants
🤖
Summary
Mobile showed every person in a DM as Offline until they happened to publish a presence update while the app was connected. Presence was never loaded when the app opened, never refreshed, and a missing or failed read looked identical to actually being offline. This PR makes presence load and stay current: on connect or foreground, the app fetches the relay's presence snapshot for everyone visible, keeps live updates overlapping so nothing is missed in between, and re-checks every 60 seconds while the app is in the foreground.
Mirrors Desktop's presence hydration (
desktop/src-tauri/src/commands/profile.rs::get_presence,desktop/src/features/presence/hooks.ts).Related issue
Testing
just mobile-checkand the full mobile test suite pass independently, and every broader CI gate has successful evidence after isolated recovery runs (no monolithic localjust cirun) — receipts in the exact-head evidence comment.Screenshots
No layout change in this slice — the renders below show the same existing profile sheet, with the presence status now following hydrated relay data. Flutter production-widget test renders — not native-device screenshots or acceptance captures.
Capture provenance
Rendered by the Flutter widget engine in a
flutter testrun (production widgets, production theme; no device or simulator). Before: this PR's declared base3c7f288c60d67df78577b237e27c3dfc8831aaa1. After: its heada8e8028bfb9bb67de2db03ed448839e2f3ab2b0d.